Add preference for visualPreview on hover #12648

sgraband · 2023-06-22T13:30:58Z

What it does

Made window.tabbar.enhancedPreview an enum preference with these options:

classic: a simple unstyled preview, containing the name
enhanced: a styled preview containing title and caption (Add extended tab bar preview option #12350)
visual: the enhanced preview + visual preview of the view Extended the hover service so that it supports the visual preview.

Added the PreviewableWidget interface.
Widgets, implementing this interface, can be previewed once they were loaded. The loaded flag is set to true when a widget is set to active. Widgets implementing the interface can specify how the preview should look like. The default is simply the node of the widget.
Webviews are currently not previewable, as they raise some challenges. For example, scripts in the webviews would need to be handled/blocked. Therefore, an approach for webviews should be tackled in a follow up.

Fixes #12646

Contributed on behalf of STMicroelectronics

How to test

Check out this PR an open Theia
Set the window.tabbar.enhancedPreview preference to visual
Observer, that on horizontal tab bars when hovering a tab of a view, that is currently not visible, but was loaded before, a visual preview is rendered in the hover feedback.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

sgraband · 2023-06-27T12:45:23Z

@vince-fugnitto Could you review this? Since you reviewed the previous PR (#12350)?

(I will resolve the Changelog conflict together with any review feedback ;))

vince-fugnitto

Thank you for the contribution, I've noticed a couple of things:

the feature does not work well with different layouts, and often hides the visual hover
the styling of the visual preview can be enhanced to have a more fixed width so it is consistent for different tabs and for different nested files (at the moment the size is quite big and hides a lot of information as opposed to the ones present in the browser, and depending where a file is located will differ in size giving an odd user-experience)

packages/core/src/browser/core-preferences.ts

packages/core/src/browser/widgets/previewable-widget.ts

sgraband · 2023-07-11T07:26:52Z

Thank you for the feedback @vince-fugnitto! I will take a look at the styling and the comments you mentioned.

sgraband · 2023-07-14T13:21:14Z

@vince-fugnitto I added three commits:
ea5397a addresses your minor feedback in the comments.
39be6ce fixes the aspect ratio of the preview windows to 16:9. This should help to unify the hover experience between views.
b30b06f fixes a small bug with the title and caption so that they acknowledge the max-width of the hover.

I am not sure, if the width problem should (or needs to) be solved with this PR. The way it is now, adaptors can set the --theia-hover-max-width css variable to the maximum width they like for the hover. The visual preview takes this max-width into account. And with the latest commit, as mentioned before, the caption and title will also acknowledge the max-width. So this is already customizable, but maybe could be improved in the future via a preference for example.

Could you take a look at the changes and tell me what you think?

vince-fugnitto · 2023-07-17T13:07:27Z

I am not sure, if the width problem should (or needs to) be solved with this PR. The way it is now, adaptors can set the --theia-hover-max-width

@sgraband my comment was more for the default user-experience and not so much the customization for downstream adopters. At the moment, depending on how long the file-path is, the preview will differ in size for different files which gives off a bad user-experience. As the default I believe that previews should have a standard size, and downstream can customize if they want to (this is similar in behavior to browser previews which the feature is inspired by).

preview-irregular-sizes.mp4

sgraband · 2023-07-18T13:00:54Z

@vince-fugnitto I changed the max-width variable to a simple width one and fixed the value to 300px. Let me know what you think.

sgraband · 2023-07-25T12:40:45Z

@vince-fugnitto would you have time to take another look this week? I will be out of office next week, so it would be great to have some feedback this week. TIA

vince-fugnitto · 2023-07-25T13:22:01Z

packages/core/src/browser/style/hover-service.css

@@ -17,7 +17,7 @@
 /* Adapted from https://github.com/microsoft/vscode/blob/7d9b1c37f8e5ae3772782ba3b09d827eb3fdd833/src/vs/workbench/services/hover/browser/hoverService.ts */

 :root {
-  --theia-hover-max-width: 500px;
+  --theia-hover-width: 300px;


I believe this is another breaking change that adopters might depend on?
I don't think there is harm in keeping both variables (one for width and one for max-width).

vince-fugnitto · 2023-07-25T13:22:39Z

packages/core/src/browser/style/hover-service.css

@@ -27,7 +27,7 @@
  background-color: var(--theia-editorHoverWidget-background);
  border: 1px solid var(--theia-editorHoverWidget-border);
  padding: var(--theia-ui-padding);
-  max-width: var(--theia-hover-max-width);
+  width: var(--theia-hover-width);


This change actually breaks other hovers we have in the framework (ex: statusbar).
Can we instead only style the enhanced preview with the proper width and not update the more generic theia-hover:

statusbar-hover.mp4

Good catch! I reworked the last commit, so that both variables are there, but the --theia-hover-preview-width variable is only used by the preview hovers.

msujew

Seems to work pretty well. I noticed some unexpected behavior when the widget has been stretched across the screen, see here. However, I believe that's due to the general approach of re-rendering the widget in another HTML element, and cannot be easily addressed. It might also just be me, I've been hoping that the preview shows more than what is rendered in the widget itself.

Anyway, aside from that I just have a minor comment. Otherwise this looks pretty good 👍

packages/core/src/browser/shell/tab-bars.ts

Made window.tabbar.enhancedPreview an enum preference with these options: - classic: a simple unstyled preview, containing the name - enhanced: a styled preview containing title and caption (eclipse-theia#12350) - visual: the enhanced preview + visual preview of the view Extended the hover service so that it supports the visual preview. Added the `PreviewableWidget` interface. Widgets, implementing this interface, can be previewed once they were loaded. The loaded flag is set to true when a widget is set to active. Widgets implementing the interface can specify how the preview should look like. The default is simply the node of the widget. Webviews are currently not previewable, as they raise some challenges. For example, scripts in the webviews would need to be handled/blocked. Therefore, an approach for webviews should be tackled in a follow up. Fixes eclipse-theia#12646 Contributed on behalf of STMicroelectronics

Adjust changelog and preference description Contributed on behalf of STMicroelectronics

This way the size of the previews is more uniform. If the view is wider than 16:9 the scaling is according to the width. If the view is longer than 16:9 the scaling is according to the height. The other dimension is then cropped. Contributed on behalf of STMicroelectronics

Before the two fields ignored the max-width. Contributed on behalf of STMicroelectronics

Contributed on behalf of STMicroelectronics

Otherwise, the `title` is missing. Ref: eclipse-theia/theia#12648

sgraband mentioned this pull request Jun 22, 2023

Add preference for visualPreview on hover eclipsesource/theia#83

Closed

1 task

sgraband force-pushed the visualPreview branch from 122df98 to fe3e7eb Compare June 22, 2023 13:32

JonasHelming requested a review from vince-fugnitto June 28, 2023 07:05

vince-fugnitto reviewed Jul 5, 2023

View reviewed changes

packages/core/src/browser/core-preferences.ts Show resolved Hide resolved

packages/core/src/browser/core-preferences.ts Outdated Show resolved Hide resolved

packages/core/src/browser/widgets/previewable-widget.ts Outdated Show resolved Hide resolved

vince-fugnitto added proposal feature proposals (potential future features) core issues related to the core of the application ui/ux issues related to user interface / user experience labels Jul 6, 2023

sgraband force-pushed the visualPreview branch from fe3e7eb to b30b06f Compare July 14, 2023 13:14

sgraband requested a review from vince-fugnitto July 14, 2023 13:21

vince-fugnitto reviewed Jul 25, 2023

View reviewed changes

vince-fugnitto requested a review from msujew July 25, 2023 13:23

sgraband force-pushed the visualPreview branch 2 times, most recently from f77f965 to 00f3a90 Compare July 25, 2023 15:19

sgraband requested a review from vince-fugnitto July 25, 2023 15:20

msujew approved these changes Jul 26, 2023

View reviewed changes

packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved

sgraband added 6 commits July 26, 2023 15:52

Address review feedback

79d48ac

Adjust changelog and preference description Contributed on behalf of STMicroelectronics

Fix small issue with hover-title and -caption css

0b32736

Before the two fields ignored the max-width. Contributed on behalf of STMicroelectronics

Fix preview width to 300px

8c388c0

Contributed on behalf of STMicroelectronics

Rename div's css class

5523c1e

Contributed on behalf of STMicroelectronics

sgraband force-pushed the visualPreview branch from 00f3a90 to 5523c1e Compare July 26, 2023 13:57

msujew merged commit 002fd17 into eclipse-theia:master Jul 27, 2023
6 checks passed

vince-fugnitto added this to the 1.40.0 milestone Jul 27, 2023

kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Sep 14, 2023

fix(regression): core preference is not optional

d4b6a0d

Otherwise, the `title` is missing. Ref: eclipse-theia/theia#12648

kittaakos pushed a commit to arduino/arduino-ide that referenced this pull request Sep 29, 2023

fix(regression): core preference is not optional

28e00dd

Otherwise, the `title` is missing. Ref: eclipse-theia/theia#12648

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preference for visualPreview on hover #12648

Add preference for visualPreview on hover #12648

sgraband commented Jun 22, 2023

sgraband commented Jun 27, 2023

vince-fugnitto left a comment

sgraband commented Jul 11, 2023

sgraband commented Jul 14, 2023

vince-fugnitto commented Jul 17, 2023

sgraband commented Jul 18, 2023

sgraband commented Jul 25, 2023

vince-fugnitto Jul 25, 2023

vince-fugnitto Jul 25, 2023

sgraband Jul 25, 2023

msujew left a comment

Add preference for visualPreview on hover #12648

Add preference for visualPreview on hover #12648

Conversation

sgraband commented Jun 22, 2023

What it does

How to test

Review checklist

Reminder for reviewers

sgraband commented Jun 27, 2023

vince-fugnitto left a comment

Choose a reason for hiding this comment

sgraband commented Jul 11, 2023

sgraband commented Jul 14, 2023

vince-fugnitto commented Jul 17, 2023

sgraband commented Jul 18, 2023

sgraband commented Jul 25, 2023

vince-fugnitto Jul 25, 2023

Choose a reason for hiding this comment

vince-fugnitto Jul 25, 2023

Choose a reason for hiding this comment

sgraband Jul 25, 2023

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment